Add managed decimal support (as in the rust framework) #477

danielailie · 2024-09-03T12:10:39Z

Added support for ManagedDecimal and ManagedDecimalSigned.

See:

managed decimal new page mx-docs#824

popenta · 2024-09-05T11:02:53Z

src/smartcontracts/typesystem/types.ts

@@ -10,18 +10,20 @@ export class Type {

    private readonly name: string;
    private readonly typeParameters: Type[];
-    protected readonly cardinality: TypeCardinality;
+    protected readonly metadata: any;


maybe move the protected member last so that they are grouped by access modifiers?

also, as discussed, the type can be string.

popenta · 2024-09-05T11:05:06Z

src/smartcontracts/typesystem/types.ts

@@ -64,10 +66,18 @@ export class Type {
        return this.typeParameters;
    }

+    getMetadata(): any {


return type should be string.

popenta · 2024-09-05T11:19:28Z

src/abi/typeFormula.ts

@@ -1,18 +1,21 @@
 export class TypeFormula {
    name: string;
+    metadata: any;


maybe change type to string here, as well

here we will let any because is the general object

Don't you think it would be better to use object type instead?

it can be also a primitive that's why I think any is a better option here

popenta · 2024-09-05T11:22:26Z

src/abi/typeFormula.ts

+        const typeParameters = hasTypeParameters
+            ? `<${this.typeParameters.map((tp) => tp.toString()).join(", ")}>`
+            : "";
+        const baseName = `${this.name}${typeParameters}`;


did you intentionally left out the <>? It used to be: return ${this.name}<${typeParameters}>;

<> is already added when we build the variable typeParameters <${this.typeParameters.map((tp) => tp.toString()).join(", ")}>

popenta · 2024-09-05T11:38:07Z

src/smartcontracts/codec/managedDecimal.ts

+        const payload = buffer.slice(0, length);
+
+        const result = this.decodeTopLevel(payload, type);
+        const decodedLength = length;


length can simply be used instead of creating a new variable, but not a big deal.

popenta · 2024-09-05T12:03:26Z

src/smartcontracts/nativeSerializer.ts

+            }
+            return new ManagedDecimalValue(native[0], native[1]);
+        }
+        errorContext.throwError(`(function: toEnumValue) unsupported native type ${typeof native}`);


I think it should be (function: toManagedDecimal)

popenta · 2024-09-05T12:34:55Z

src/smartcontracts/interaction.local.net.spec.ts

@@ -184,6 +185,118 @@ describe("test smart contract interactor", function () {
        assert.isTrue(typedBundle.returnCode.equals(ReturnCode.Ok));
    });

+    it("should interact with 'basic-features' (local testnet) using the SmartContractTransactionsFactory", async function () {


not using SmartContractTransactionsFactory.

andreibancioiu · 2024-09-05T11:55:12Z

src/smartcontracts/codec/managedDecimal.ts

+
+    decodeTopLevel(buffer: Buffer, type: ManagedDecimalType): ManagedDecimalValue {
+        if (buffer.length === 0) {
+            return new ManagedDecimalValue(new BigNumber(0), 2);


Magic number 2 can be a constant - is it default scale of managed decimal?

andreibancioiu · 2024-09-05T11:56:32Z

src/smartcontracts/codec/managedDecimal.ts

+        const isUsize = type.getMetadata() === "usize";
+
+        if (isUsize) {
+            const u32Size = 4;


We have a constant here:

https://github.com/multiversx/mx-sdk-js-core/blob/main/src/smartcontracts/codec/constants.ts

andreibancioiu · 2024-09-05T11:58:39Z

src/smartcontracts/codec/managedDecimal.ts

+
+    encodeNested(value: ManagedDecimalValue): Buffer {
+        let buffers: Buffer[] = [];
+        if (value.getType().getMetadata() == "usize") {


Maybe it's worth to add a function on ManagedDecimalValue, such as isVariable()?

Can also say (optional):

const valueBuffer = uffer.from(this.binaryCodec.encodeTopLevel(new BigUIntValue(value.valueOf()); if (!value.isVariable) { return valueBuffer; } const scaleBuffer = Buffer.from(this.binaryCodec.encodeNested(new U32Value(value.getScale())); return Buffer.concat([...]);

this is not working becauze if is not variable the value is encodet top level if is variable the value is encodedNested

done, I add it on type and use it here

andreibancioiu · 2024-09-05T11:59:27Z

src/smartcontracts/codec/managedDecimal.ts

+
+        const isUsize = type.getMetadata() === "usize";
+
+        if (isUsize) {


Is it worth to add maybe a function on ManagedDecimalType, such as type.isVariable()?

andreibancioiu · 2024-09-05T12:07:55Z

src/smartcontracts/typesystem/types.ts

    isGenericType(): boolean {
        return this.typeParameters.length > 0;
    }

+    hasMetadata(): boolean {
+        return this.metadata !== null && this.metadata !== undefined;


Alternatively, !! operator 🙈 (the "bang, bang, you're a boolean operator").

andreibancioiu · 2024-09-05T12:45:06Z

src/smartcontracts/typesystem/managedDecimal.ts

+            return false;
+        }
+
+        return this.value == other.value;


Maybe also compare scale?

we don't need to compare the scale in order to compare the values, I added some tests to test it

andreibancioiu · 2024-09-05T12:45:26Z

src/smartcontracts/typesystem/managedDecimal.ts

+            return false;
+        }
+
+        return this.value == other.value;


Does this work when comparing instances of BigNumber?

yes, it works

andreibancioiu · 2024-09-05T12:46:27Z

src/smartcontracts/typesystem/managedDecimal.ts

+    /**
+     * Returns whether two objects have the same value.
+     */
+    equals(other: ManagedDecimalSignedValue): boolean {


Same questions as above.

andreibancioiu · 2024-09-05T12:47:31Z

src/smartcontracts/typesystem/managedDecimal.ts

+    }
+
+    toString(): string {
+        return this.value.toFixed(this.scale);


Let's have some small unit tests for ManagedDecimalValue and ManagedDecimalSignedValue.

andreibancioiu · 2024-09-05T12:59:52Z

src/smartcontracts/typesystem/typeMapper.ts

-        this.learnedTypesMap.delete(type.getName());
-        this.learnedTypesMap.set(type.getName(), type);
+        if (type.getName() === "ManagedDecimal" || type.getName() === "ManagedDecimalSigned") {
+            this.learnedTypesMap.delete(type.getName());


As discussed separately, fix is needed here.

popenta · 2024-09-09T15:08:59Z

src/smartcontracts/typesystem/types.ts

    }

    private getFullNameForGeneric(): string {
-        const hasTypeParameters = this.getTypeParameters.length > 0;
+        const hasTypeParameters = this.getTypeParameters().length > 0;


this.isGenericType() could have been used, but this does the same thing.

andreibancioiu

The equals() function should be fixed. All the others are optional comments and ideas.

andreibancioiu · 2024-09-09T15:04:10Z

src/abi/typeFormulaParser.ts

@@ -81,9 +81,10 @@ export class TypeFormulaParser {
    private acquireTypeWithParameters(stack: any[]): TypeFormula {
        const typeParameters = this.acquireTypeParameters(stack);
        const typeName = stack.pop();
+        const metadata = typeParameters[0].name;


To be more explicit that we deal with metadata only for some types, variable should be defined under the scope of if managed decimal (I know it works the same).

Optional, can also stay as it is.

andreibancioiu · 2024-09-09T15:07:12Z

src/smartcontracts/codec/managedDecimal.ts

    }

    encodeNested(value: ManagedDecimalValue): Buffer {
        let buffers: Buffer[] = [];
-        if (value.getType().getMetadata() == "usize") {
+        const managedDecimalType = value.getType() as ManagedDecimalType;


If we add isVariable() not only to the type, but also to the value, we work around without type assertions.

andreibancioiu · 2024-09-09T15:08:06Z

src/smartcontracts/typesystem/managedDecimal.ts

+        return this.metadata;
+    }
+
+    isVariable(): boolean {


Useful both on type and value.

andreibancioiu · 2024-09-09T15:13:22Z

src/smartcontracts/typesystem/managedDecimal.ts

-    constructor(value: BigNumber.Value, scale: number) {
-        super(new ManagedDecimalType(scale));
+    constructor(value: BigNumber.Value, scale: number, isVariableDecimals: boolean = false) {
+        super(new ManagedDecimalSignedType(isVariableDecimals ? "usize" : scale));


Also in ManagedDecimalValue - is it worth to encapsulate this tricky logic somewhere? E.g. named constructor of the types? 🙈

andreibancioiu · 2024-09-09T15:14:04Z

src/smartcontracts/typesystem/managedDecimal.ts

@@ -84,8 +89,8 @@ export class ManagedDecimalSignedValue extends TypedValue {
    private readonly value: BigNumber;
    private readonly scale: number;

-    constructor(value: BigNumber.Value, scale: number) {
-        super(new ManagedDecimalType(scale));
+    constructor(value: BigNumber.Value, scale: number, isVariableDecimals: boolean = false) {


Parameter name isVariableDecimals is not consistent with parameter name of ManagedDecimalValue constructor. Maybe isVariable for both of them?

andreibancioiu · 2024-09-09T15:32:10Z

src/smartcontracts/typesystem/typeExpressionParser.ts

@@ -26,6 +26,6 @@ export class TypeExpressionParser {

    private typeFormulaToType(typeFormula: TypeFormula): Type {
        const typeParameters = typeFormula.typeParameters.map((typeFormula) => this.typeFormulaToType(typeFormula));
-        return new Type(typeFormula.name, typeParameters, undefined, typeFormula.metadata);
+        return new Type(typeFormula.name, typeParameters, TypeCardinality.fixed(1), typeFormula.metadata);


I was wrong before. Maybe undefined is better here (if all tests still pass). Cardinality == 1 is tricky (it's a white lie), since here we also create & return variadic types.

Can be either undefined of fixed(1) - since the latter is the default anyway. When applying the Python-like simplifications (in the not very close future), this will get simplified (no cardinality abstraction etc.).

andreibancioiu · 2024-09-09T15:35:13Z

src/smartcontracts/typesystem/managedDecimal.spec.ts

+        let firstValue = new ManagedDecimalValue(new BigNumber(1), 2, false);
+        let secondValue = new ManagedDecimalValue(new BigNumber(2), 2, false);


Here / above / below - can also be const.

andreibancioiu · 2024-09-09T15:35:22Z

src/smartcontracts/typesystem/managedDecimal.spec.ts

+    });
+
+    it("should set the correct scale when variable decimals", () => {
+        let value = new ManagedDecimalValue(new BigNumber(1.3), 2, true);


Sorry for not thinking of this before - how about:

new ManagedDecimalValue("42.43", 8); new ManagedDecimalValue("42.43", "usize");

Using the trick type ManagedDecimalScale = number | "usize".

andreibancioiu · 2024-09-09T15:36:13Z

src/smartcontracts/typesystem/managedDecimal.spec.ts

+        let firstValue = new ManagedDecimalValue(new BigNumber(1.234), 3, false);
+        let secondValue = new ManagedDecimalValue(new BigNumber(12.34), 2, false);
+
+        assert.isFalse(firstValue.equals(secondValue));


Let's have a "positive" equality test / equal values, as well.

andreibancioiu · 2024-09-09T15:39:20Z

src/smartcontracts/typesystem/managedDecimal.spec.ts

+    });
+
+    it("should compare correctly two managed decimals even with different scale", () => {
+        let firstValue = new ManagedDecimalValue(new BigNumber(1.234), 3, false);


For documentation purposes, when a BigNumber is created from non-integers, let's pass them as a string. Maybe in our examples, drop the BigNumber wrapping e.g.

new ManagedDecimalValue("1.234", 3, false);

Instead of:

new ManagedDecimalValue(new BigNumber(...), 3, false);

Since in the constructor of ManagedDecimalValue this is handled correctly anyway.

danielailie added 11 commits August 26, 2024 09:53

Add managed decimal

615034c

Extend managed Decimal

c59657d

Add basic feature abi and update tests

ad8d581

Clean up colsole logs

ab476cf

Add encoded nested for managed decimal

9dceb18

refactor managed decimal codec

06b85fe

Add tests for abi registry managed decimals

e35ad8f

remove unused tests

6065dee

Update native serializer

0d9e486

Update native serializer tests

5a11659

Merge branch 'main' into TOOL-223-add-managed-decimal-support-from-rust

42bf1c6

danielailie changed the title ~~[WIP] Tool 223 add managed decimal support from rust~~ Tool 223 add managed decimal support from rust Sep 5, 2024

danielailie self-assigned this Sep 5, 2024

popenta self-requested a review September 5, 2024 11:00

popenta reviewed Sep 5, 2024

View reviewed changes

andreibancioiu self-requested a review September 5, 2024 11:24

popenta reviewed Sep 5, 2024

View reviewed changes

danielailie added 3 commits September 5, 2024 15:42

Fix integretion tests

dada969

Core review follow up

73670bd

Remove unused variable

1f65ffe

andreibancioiu reviewed Sep 5, 2024

View reviewed changes

danielailie added 4 commits September 5, 2024 16:38

code review follow up

8e054c6

Add tests for managed decimals

9135302

Remove console logs

33a875d

Fixed failing test

14b2f5f

CiprianDraghici previously approved these changes Sep 9, 2024

View reviewed changes

popenta reviewed Sep 9, 2024

View reviewed changes

popenta previously approved these changes Sep 9, 2024

View reviewed changes

andreibancioiu reviewed Sep 9, 2024

View reviewed changes

Code review follow up

275bb49

danielailie dismissed stale reviews from popenta and CiprianDraghici via 275bb49 September 10, 2024 09:11

danielailie added 2 commits September 10, 2024 14:50

Fix test name

0ca88e9

remove only

3838dc7

andreibancioiu approved these changes Sep 10, 2024

View reviewed changes

CiprianDraghici approved these changes Sep 10, 2024

View reviewed changes

popenta approved these changes Sep 10, 2024

View reviewed changes

danielailie merged commit b8927c6 into main Sep 10, 2024
3 checks passed

danielailie deleted the TOOL-223-add-managed-decimal-support-from-rust branch September 10, 2024 12:20

andreibancioiu changed the title ~~Tool 223 add managed decimal support from rust~~ Add managed decimal support (as in the rust framework) Sep 10, 2024


		const isUsize = type.getMetadata() === "usize";

		if (isUsize) {

		let firstValue = new ManagedDecimalValue(new BigNumber(1), 2, false);
		let secondValue = new ManagedDecimalValue(new BigNumber(2), 2, false);

Add managed decimal support (as in the rust framework) #477

Add managed decimal support (as in the rust framework) #477

Conversation

danielailie commented Sep 3, 2024 • edited by andreibancioiu Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreibancioiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreibancioiu Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreibancioiu Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreibancioiu Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

danielailie commented Sep 3, 2024 •

edited by andreibancioiu

Loading

andreibancioiu Sep 9, 2024 •

edited

Loading

andreibancioiu Sep 9, 2024 •

edited

Loading

andreibancioiu Sep 9, 2024 •

edited

Loading